fix(flatkv): report snapshots a rollback could not remove - #3887
Conversation
Rollback promises that no snapshot beyond its target survives, but the final removal step dropped both the traversal error and each per-directory error, so it logged "Rollback complete" and returned nil with a future snapshot still on disk. An operator running `seid rollback`, or startup reconciliation, was told the rewind was clean when it was not. Extract the step as removeSnapshotsAbove and return its error, naming the directory left to reconcile. pruneSnapshots then compounded it: treating every version != currentVersion as "old" let such a remnant take a keep slot, evicting a genuinely older snapshot that rollback still needs as a base. Restrict candidates to versions strictly below the current one, matching the guard memiavl's pruneSnapshots already applies, and log the traversal error it also dropped. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
PR SummaryMedium Risk Overview Also tightens Reviewed by Cursor Bugbot for commit 3125131. Bugbot is set up for automated code reviews on this repo. Configure here. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 245d310b10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3887 +/- ##
==========================================
- Coverage 59.47% 58.68% -0.80%
==========================================
Files 2323 2242 -81
Lines 198543 190387 -8156
==========================================
- Hits 118084 111722 -6362
+ Misses 69249 68053 -1196
+ Partials 11210 10612 -598
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Both changes are correct and well-motivated: restricting prune candidates to versions strictly below currentVersion matches memiavl's guard and the new test genuinely fails under the old v != currentVersion logic, and surfacing the removal error makes Rollback's contract honest. Remaining notes are about the accuracy of the new error message (partial removal leaves a -removing path, and traversal aborts at the first failure so other remnants go unnamed) plus the operational effect of the now-propagating error.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Behavioral change worth calling out:
Rollback's error now propagates throughCompositeCommitStore.Rollbackand the version-reconciliation path (sei-db/state_db/sc/composite/store.go:884), so a node that reaches the target version but cannot unlink a stale snapshot directory will now fail startup reconciliation instead of continuing. This looks intentional and is retryable across a restart (the WAL is already pruned andcurrentpoints at the base, so a re-run re-attempts the removal), but it converts a previously survivable filesystem hiccup into a startup failure — worth a line in the PR description or release notes for operators. - Test coverage stops at the helper:
TestRemoveSnapshotsAboveKeepsTargetAndBelowandTestRemoveSnapshotsAboveReportsFailurecoverremoveSnapshotsAbovedirectly, but nothing asserts thatRollbackitself now returns (rather than swallows) that error — which is the actual regression the PR describes. The existingTestRollbackRemovesPostTargetSnapshotscould be paired with a variant that makes the removal fail and assertsRollbackreturns an error naming the leftover directory. - The Cursor second-opinion file (
./cursor-review.md) is empty — that pass produced no output, so its perspective is not represented in this consolidated review. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
Returning the removal error from the end of Rollback made the error unactionable. By that point the WAL and both databases have already reached the target, but the error aborts CompositeCommitStore.Rollback before it resets the commit-info latches and rootmulti.RollbackToVersion before it refreshes lastCommitInfo. Worse, `seid rollback` rewinds the app before Tendermint, so failing in between leaves the app at the target and consensus above it. Run the removal before the WAL prune instead. An error now means the rollback did not take effect, so every caller that skips its post-rollback bookkeeping on error is correct to, and a restart replays the un-pruned WAL back to the old tail so the rollback can be retried. Also attempt every candidate rather than stopping at the first failure, joining the errors as removeTmpDirs already does, since the caller needs the whole list to reconcile from. The remediation text is dropped from the message: atomicRemoveDir renames before unlinking, so a partial removal leaves snapshot-N-removing rather than the snapshot-N the text named, and removeTmpDirs sweeps that on the next open regardless. The new ordering makes the end-to-end path testable: with the removal ahead of open(), a blocked trash directory survives removeTmpDirs long enough to fail the removal, so a test can assert Rollback reports it and leaves the WAL intact. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A well-targeted correctness fix: Rollback no longer reports success while a snapshot above the target survives, and pruneSnapshots no longer lets such a remnant take a keep slot and evict a genuinely older snapshot (matching memiavl's version >= currentVersion guard, which I verified at sei-db/state_db/sc/memiavl/db.go:572). No blockers; the notes below are about a doc-comment invariant that is overstated, error-message consistency, and a pre-existing composite-rollback gap that Codex surfaced.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex's point, kept with a downgrade:
CompositeCommitStore.Rollback(sei-db/state_db/sc/composite/store.go) rolls back memIAVL first and flatKV second, with no compensation if the second fails after the first succeeded — memIAVL stays rewound while flatKV self-heals to its old tail on restart. That hazard already exists for every other error return inflatKV.Rollback(closeDBsOnly, updateCurrentSymlink, SNAPSHOT_BASE removal, open, WAL close/prune/reopen, catchup, version mismatch), so this PR adds one more instance rather than a new class — and the alternative (keep swallowing the error) is exactly the bug being fixed. Worth a follow-up on the composite's two-phase ordering; not a reason to hold this change. pruneSnapshots's new traversal-error branch (returns 0 and logs) has no test. The two new prune/remove tests cover the happy paths and the per-directory failure path, but not thetraverseSnapshotsfailure that the diff newly stops ignoring.- With
pruneSnapshotsnow restricted tov < currentVersion, a remnant above the current version has no dedicated reclaim path — it is only cleared by a later successfulRollback, or overwritten once the chain re-reaches that version (WriteSnapshot'satomicRemoveDir(finalPath)at snapshot.go:497). That is the right trade-off and the PR documents it, but it is worth stating in the doc comment that the remnant lingers rather than being collected, so a reader does not assume something else sweeps it. - The Cursor second-opinion file (
./cursor-review.md) is empty — that pass produced no output, so this review reflects only the Claude and Codex passes. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| // A failure here is returned rather than logged, which is why the step runs before the WAL is pruned: an | ||
| // error then means the rollback did not take effect, so a caller that skips its own post-rollback bookkeeping | ||
| // on error is right to. `seid rollback` in particular rewinds the app before Tendermint, and aborting between | ||
| // those two would leave the two heights apart; running here it aborts before either moves. Reporting success |
There was a problem hiding this comment.
[suggestion] This invariant is overstated: by the time this step runs, things have moved.
CompositeCommitStore.RollbackcallsmemIAVL.Rollback(targetVersion)beforeflatKV.Rollback(targetVersion), so the cosmos stores are already rewound when this returns an error.- Within flatkv itself,
closeDBsOnly,updateCurrentSymlink(dir, snapshotName(baseVersion))and theSNAPSHOT_BASEremoval have all already run (lines 662-676).
The claim that actually holds — and it is the load-bearing one — is narrower: the WAL is untouched, so flatkv replays back to its old tail on restart, and Tendermint has not been rolled back yet because the CLI does the app first (sei-cosmos/server/rollback.go, rollbackAppState before rollbackTendermintState). Per AGENTS.md the doc comment is where the why lives, so it is worth narrowing "before either moves" to that, and saying explicitly that memIAVL is left rewound.
| } | ||
|
|
||
| if err := removeSnapshotsAbove(dir, targetVersion); err != nil { | ||
| return err |
There was a problem hiding this comment.
[suggestion] Bare return err here is the only mid-rollback error return that does not name the store's state. The WAL-window errors just below (lines ~698-712) all end with "store is mid-rollback, restart to recover then retry", and the earlier steps wrap with "... for rollback". At this point the DBs are closed, current already points at snapshot %d and SNAPSHOT_BASE is gone — the operator needs the same restart-then-retry hint, and the store's identity in the message. Suggest wrapping consistently, e.g.:
if err := removeSnapshotsAbove(dir, targetVersion); err != nil {
return fmt.Errorf("rollback to version %d (from snapshot %d): %w; "+
"store is mid-rollback, restart to recover then retry", targetVersion, baseVersion, err)
}| // atomicRemoveDir renames snapshot-6 onto this trash name before unlinking it, so an undeletable | ||
| // directory already sitting there fails that rename. Restore permissions before t.TempDir's own cleanup, | ||
| // which runs after this one. | ||
| blocker := filepath.Join(cfg.DataDir, snapshotName(6)+removingSuffix) |
There was a problem hiding this comment.
[nit] Good test — I traced through atomicRemoveDir (snapshot.go:314) and confirmed the mechanism works: the ignored os.RemoveAll(trashPath) cannot unlink occupied inside a 0555 dir, so the subsequent os.Rename fails on a non-empty target. That dependency on RemoveAll's error being discarded is load-bearing and a bit indirect; consider naming it in the comment so a future change to atomicRemoveDir's error handling does not silently turn this into a no-op test that passes for the wrong reason.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a81f20f. Configure here.
main renamed pruneSnapshots to pruneSnapshotsByCount and gave it an ExternalPruning stand-down (#3868), touching the same two regions of flatkv/snapshot.go this branch changes. Resolution: keep main's name and its ExternalPruning guard, and keep this branch's candidate rule (only snapshots strictly below currentVersion) inside it, so the by-count path and the collector's PruneSnapshots both refuse to delete a snapshot above the active one. removeSnapshotsAbove and main's rewritten tryTruncateWAL comment are independent and both kept. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Solid, well-motivated fix: returning the snapshot-removal error and moving that step ahead of the WAL prune closes a real "reported clean, remnant on disk" bug, and restricting pruneSnapshotsByCount to versions strictly below current stops a remnant from evicting a rollback base. No blocking issues; the remaining notes are godoc style (per AGENTS.md), a slightly overstated invariant in a comment, and test-coverage robustness.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion review file (
cursor-review.md) is empty — that pass produced no output, so this review reflects only the Codex findings plus my own. tryTruncateWAL(sei-db/state_db/sc/flatkv/snapshot.go:777) still discards thetraverseSnapshotserror with_ =, the exact pattern this PR fixes at line 551. It fails safe today (earliestSnapVersionstays 0 and the function returns early, skipping the prune), so this is a consistency nit rather than a bug — but the WAL then silently stops being truncated, which is worth a log line at minimum. Out of scope for this PR if you'd rather keep it focused.- Behaviour worth confirming: with the
<guard, a snapshot abovecurrentVersionis now never reclaimed by count-based pruning at all. It's bounded (it becomes a candidate again once the chain advances past it) and rollback now errors rather than leaving one behind, so this looks like the right trade — just noting the leak window is intentional. - No test covers the new
traverseSnapshotserror path inpruneSnapshotsByCount(log + return 0). Low value given it's only reachable on aReadDirfailure, but it is newly added behaviour. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| // removeSnapshotsAbove deletes every snapshot directory above targetVersion. | ||
| // | ||
| // A failure here is returned rather than logged, which is why the step runs before the WAL is pruned: an |
There was a problem hiding this comment.
[suggestion] Two notes on this godoc.
Style (also raised by Codex, and applies to the additions at line 530 and 643-649): AGENTS.md asks that godocs explain what, not why or how, that multi-paragraph godocs be rare, and that rationale live in an inline comment at the line that needs it. This is three paragraphs of pure rationale plus a tour of seid rollback's caller behaviour. The file is already written in this register, so I'd treat it as a nit — but the ordering rationale in particular reads better as an inline comment at the call site (line 685), where the ordering constraint actually lives, leaving the godoc as the one-line first sentence.
Precision: "an error then means the rollback did not take effect" is stronger than what the code guarantees. By the time this runs, updateCurrentSymlink has already repointed current at baseVersion and SNAPSHOT_BASE has been removed, so the store is mid-rollback in-process; convergence back to the old head depends on a restart replaying the un-pruned WAL. And in CompositeCommitStore.Rollback (composite/store.go:1231), memIAVL.Rollback has already committed durably before flatKV.Rollback is reached, so on this error the two backends are genuinely split until startup reconciliation runs. The property you actually have — "no durable rewind is committed; a restart replays back to the old tail and the rollback can be retried" — is what the PR description says and is worth stating that way here.
| } | ||
|
|
||
| func TestRemoveSnapshotsAboveReportsEveryFailure(t *testing.T) { | ||
| if os.Geteuid() == 0 { |
There was a problem hiding this comment.
[suggestion] Both new failure tests (this one and TestRollbackReportsUnremovableSnapshotWithoutRewinding at line 1260) skip entirely when running as root. CI Go jobs in containers commonly run as uid 0, in which case the two tests that pin the actual new behaviour — error propagation and the removal-before-WAL-prune ordering — silently contribute nothing, and only TestRemoveSnapshotsAboveKeepsTargetAndBelow and the prune test still guard the change.
Worth checking whether this repo's go-test.yml runs as root. If it does, consider a failure injection that works regardless of uid — e.g. planting a file (not a directory) at snapshot-N-removing so atomicRemoveDir's os.Rename of a directory onto it fails with ENOTDIR for root too, or replacing snapshot-N itself with something rename-hostile. At minimum, t.Log on the skip so the gap is visible in CI output rather than invisible.
3 participants
Rollback promises that no snapshot beyond its target survives, but the removal step dropped both the traversal error and each per-directory error, so it logged "Rollback complete" and returned nil with a future snapshot still on disk. An operator running
seid rollback, or startup reconciliation, was told the rewind was clean when it was not.pruneSnapshotsthen compounded it: treating every version!= currentVersionas "old" let such a remnant take a keep slot, evicting a genuinely older snapshot that rollback still needs as a base.Describe your changes and provide context
Return the removal error, and run the removal before the WAL is pruned. Propagating the error on its own is not enough, which is what review caught first: at the end of
Rollbackthe WAL and both databases have already reached the target, so an error there abortsCompositeCommitStore.Rollbackbefore it resets the commit-info latches, androotmulti.RollbackToVersionbefore it refresheslastCommitInfo.seid rollbackrewinds the app before Tendermint, so failing between those two leaves the app at the target and consensus above it.Running the removal ahead of the WAL prune gives the invariant that makes the error actionable:
Every caller that skips its post-rollback bookkeeping on error is then right to, so no caller changes, and a restart replays the un-pruned WAL back to the old tail so the rollback can be retried. The cost is a cached checkpoint the next
WriteSnapshotrebuilds, never history.Attempt every candidate rather than stopping at the first failure. Halting names one directory when several may survive; the errors are joined instead, the shape
removeTmpDirsalready uses in this file. The remediation text is dropped from the message:atomicRemoveDirrenames before unlinking, so a partial removal leavessnapshot-N-removingrather than thesnapshot-Nthe text named, andremoveTmpDirssweeps that on the next open regardless.Restrict prune candidates to versions strictly below the current one, matching the guard
memiavl'spruneSnapshotsalready applies (memiavl/db.go:572), and log the traversal error it also dropped.Operator-facing note
Rollbackcan now fail where it previously logged and continued. The failure is clean and retryable: it happens before the WAL is pruned, so the store is left untouched and a restart plus a re-run converges. A node that cannot unlink a stale snapshot directory during startup reconciliation will fail to start rather than continue with a remnant on disk.Testing performed to validate your change
TestRollbackReportsUnremovableSnapshotWithoutRewindingblocks the removal with an undeletable trash directory, then assertsRollbackreports it and leaves the WAL still holding the blocks above the target. Moving the removal back to the end ofRollbackchanges the failure toopen for rollback: cleanup tmp dirs: ..., so this test pins the ordering rather than only the error.TestRemoveSnapshotsAboveReportsEveryFailurepins that a failure on the lowest candidate does not hide the others.TestPruneSnapshotsIgnoresSnapshotsAboveCurrentwas verified to fail under the old!=guard, which prunessnapshot-10.go test -race ./sei-db/state_db/sc/flatkv/...(292s), plus the rollback and reconcile tests incompositeandstorev2/rootmulti.golangci-lintreports 0 issues;gofmt -sandgoimportsclean.